Skip to content

Conversation

@Yanis002
Copy link
Collaborator

Remake of PR #56 but the code is shorter by a lot, also the XML data might not be always accurate so I'll probably do some documentation (which I did a bit)

Anyway, tl;dr this is basically that tool but in Fast64, and this is a draft as most features are done, I just forgot to make a small upgrade logic and also the UI for entrances and transition actors

@Yanis002 Yanis002 marked this pull request as ready for review July 24, 2023 15:00
@Yanis002
Copy link
Collaborator Author

this should be ready to review now, issues will most likely be caused by the XML documentation but it should be fine

"""Returns the parameter with the correct format"""
shift = getShiftFromMask(mask) if mask is not None else 0

if not int(getEvalParams(value), base=16):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what this means, you want to check for 0 ? == 0 would be clearer

also getEvalParams should be modified to return an int instead of a string

or do something like:

def getEvalParamsInt(...):
    ... current getEvalParams code minus stringify
def getEvalParams(...):
    i = getEvalParamsInt(...)
    return f"{i:X}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift = shift_from_mask if there's a mask else if there's no mask then there's no shift

Comment on lines 166 to 174

actorParam: StringProperty(name="Actor Parameter", default="0x0000")
rotOverride: BoolProperty(name="Override Rotation", default=False)
rotOverrideX: StringProperty(name="Rot X", default="0")
rotOverrideY: StringProperty(name="Rot Y", default="0")
rotOverrideZ: StringProperty(name="Rot Z", default="0")
rotOverrideX: StringProperty(name="Rot X", default="0x0000")
rotOverrideY: StringProperty(name="Rot Y", default="0x0000")
rotOverrideZ: StringProperty(name="Rot Z", default="0x0000")

params: StringProperty(
name="Actor Parameter",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could there be a comment somewhere about the difference between actorParam and params

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actorParam is used for custom actors (actors with the id custom)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both are a StringProperty without get/set, it doesn't seem there needs to be two?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params use get/set

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's upgrade code but no cur_version bump anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeaaaaahhhhhh I guess, that PR is kinda old now so I probably didn't know yet about that and end up forgetting about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since I remembered why, cur_version doesn't exist anymore. we discussed about that a while ago and figured out checking if old props exist in a blend is stronger than a version check (idr the specifics though, iirc it was kure's idea)

Comment on lines 748 to 750
override = "Override" if actorProp.actorID == "Custom" else ""
overrideRots = [getattr(actorProp, f"rot{override}{rot}") for rot in ["X", "Y", "Z"]]
exportRots = [f"DEG_TO_BINANG({(rot * (180 / 0x8000)):.3f})" for rot in blendRots]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so there are three sets of rotations?

  • one from the blender object
  • two "override", one for non-custom actors and one for custom? why are these two different? why are the properties for "Custom" named "Override"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • blender object (which won't be used if the actor can have parameters in its rotation, which makes sense to me)
  • string prop rotation for actors without the id "custom"
  • string prop rotation for custom actors

iirc the override thing is gone and it all depend on the xml data, I fail to see how using rotations as real rotation and not parameter can be useful if the actor is using rotations

btw this depends on other parameters, some actors doesn't always actually check for the rotation for params, in this case the blender object rotation can be used (aka if you don't see any rotation param field in the fast64 ui)

@Reonu
Copy link
Contributor

Reonu commented Apr 18, 2024

@Dragorn421 I'm using this rn and it works great, it makes oot scene creation 100 times easier and it's a crime that it's not merged :(

@Yanis002
Copy link
Collaborator Author

Yanis002 commented Apr 19, 2024

(just so I don't forget) todo: look into upgrade issues

edit: apparently it works?? I'm confused

@Yanis002 Yanis002 added this to the v2.2.2 milestone May 20, 2024
Copy link
Contributor

@Reonu Reonu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this get merged soon? I've been using it for quite some time now and it seems to have no issues (the upgrade thing was seemingly a false alarm too).

Giving it an approving review since I've been using it a lot and I can confirm it works well

@Yanis002
Copy link
Collaborator Author

I added this to the next milestone a while ago so it will be merged before september for sure (hopefully)

also I don't remember if it was this PR specifically but I think I had another issue but I can't remember what is was 🥲

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems mostly good. However, I think Type/Enum should support custom values (ex. you want to add a new Poe type while still using the existing Poe actor), and parameters should be allowed to use string inputs (ex. MY_CUSTOM_FLAG instead of 0x01).

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some issues:

  1. For Treasure Chest / Navi Msg, choosing a custom chest content / message value causes an error.
  2. If eval params is enabled but the params fail to evaluate, I think the value should still be the unevaluated params. Currently its left blank, which results in an invalid export.
  3. For Rot XYZ + Params, if string input is used, I think masking / shifting should still be applied, so that its consistent with numerical input.

@Dragorn421 Dragorn421 modified the milestones: v2.3.0, v2.4.0 Aug 19, 2024
@Dragorn421 Dragorn421 added enhancement New feature or request oot Has to do with the Ocarina of Time 64 side labels Aug 20, 2024
@Yanis002
Copy link
Collaborator Author

Yanis002 commented Dec 1, 2024

I fixed the issues you mentioned, the only thing left is this: For Rot XYZ + Params, if string input is used, I think masking / shifting should still be applied, so that its consistent with numerical input. but I don't understand what you mean here

Copy link
Contributor

@Reonu Reonu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this get merged? I've been using it for a while, it works perfectly and the difference in usability for oot scene editing is insane.

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me.

@Yanis002 Yanis002 added the merge soon Will be merged in a few days at most if nothing else comes up label Jan 1, 2025
@Yanis002 Yanis002 merged commit dac0a8f into Fast-64:main Jan 1, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request merge soon Will be merged in a few days at most if nothing else comes up oot Has to do with the Ocarina of Time 64 side

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants